-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add PropertyBuilder implementation #95303
Conversation
Tagging subscribers to this area: @dotnet/area-system-reflection-emit Issue DetailsAdd PropertyBuilder implementation for new persist able reflection emit implementation. Contributes to #92975
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; some misc nits\questions.
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/FieldBuilderImpl.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs
Outdated
Show resolved
Hide resolved
|
||
if (property._otherMethods != null) | ||
{ | ||
foreach (MethodBuilderImpl method in property._otherMethods) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have tests for other property methods? IL-only tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have only this:
Lines 56 to 58 in dece90d
// Not sure how other methods should have loaded/tested | |
// 'propertyFromDisk.GetAccessors(true)' did not return other method | |
Assert.NotNull(typeFromDisk.GetMethod("OtherMethod", BindingFlags.NonPublic | BindingFlags.Instance)); |
In the generated assembly it was added just like a normal method, was not sure what else to verify
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this was for ".other" methods which are different than ".get" and ".set" (ECMA allows methods other than get\set)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is the "other" method
The failures unrelated and known |
Add PropertyBuilder implementation for new persist able reflection emit implementation.
Contributes to #92975